Fix bug where fvh fragments could be loaded from wrong doc #65641
Conversation
jtibshirani
added
>bug
:Search Relevance/Highlighting
| @@ -160,8 +160,13 @@ public HighlightField highlight(FieldHighlightContext fieldContext) throws IOExc | |||
| } | |||
| cache.fvh.setPhraseLimit(field.fieldOptions().phraseLimit()); | |||
|
|
|||
| String[] fragments; | |||
| // If the fragments builder requires document _source, pass it the source lookup from the hit context. | |||
| if (entry.fragmentsBuilder instanceof SourceBasedFragmentsBuilder) { | |||
There was a problem hiding this comment.
This is hacky but felt like the best way to fix this right now. FragmentsBuilder is an external (Lucene) interface and there's not a good way to pass in _source to its methods like getFields.
There was a problem hiding this comment.
Could we replace the cached FragmentsBuilder with a Function<SourceLookup, FragmentsBuilder> and then call that with the new SourceLookup for each hit? The FragmentsBuilders themselves don't seem to do any significant processing on construction so I don't think there would be a performance penalty for rebuilding them each time.
There was a problem hiding this comment.
It seems like that should work! I'd like to check I understand the context. We have these cached entries not because fragment builders are expensive to build, but to avoid rebuilding the FieldQuery which looks more complex?
jtibshirani
force-pushed
the
fvh
branch
3 times, most recently
from
a4b4af4 to
ab7abb6
Compare
|
Pinging @elastic/es-search (Team:Search) |
|
Thanks @romseygeek for the suggestion, I tried out the new approach. |
…5641) This PR fixes a regression where fvh fragments could be loaded from the wrong document _source. Some `FragmentsBuilder` implementations contain a `SourceLookup` to load from _source. The lookup should be positioned to load from the current hit document. However, since `FragmentsBuilder` are cached and shared across hits, the lookup is never updated to load from the new documents. This means we accidentally load _source from a different document. The regression was introduced in elastic#60179, which started storing `SourceLookup` on `FragmentsBuilder`. Fixes elastic#65533.
…5641) This PR fixes a regression where fvh fragments could be loaded from the wrong document _source. Some `FragmentsBuilder` implementations contain a `SourceLookup` to load from _source. The lookup should be positioned to load from the current hit document. However, since `FragmentsBuilder` are cached and shared across hits, the lookup is never updated to load from the new documents. This means we accidentally load _source from a different document. The regression was introduced in elastic#60179, which started storing `SourceLookup` on `FragmentsBuilder`. Fixes elastic#65533.
This PR fixes a regression where fvh fragments could be loaded from the wrong
document _source.
Some
FragmentsBuilderimplementations contain aSourceLookupto load from_source. The lookup should be positioned to load from the current hit document.
However, since
FragmentsBuilderare cached and shared across hits, the lookupis never updated to load from the new documents. This means we accidentally
load _source from a different document.
The regression was introduced in #60179, which started storing
SourceLookupon
FragmentsBuilder.Fixes #65533.